Add ProcessStartOptions class with simplified path resolution#124271
Add ProcessStartOptions class with simplified path resolution#124271
Conversation
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
|
Tagging subscribers to this area: @dotnet/area-system-diagnostics-process |
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot please address my feedback
src/libraries/System.Diagnostics.Process/tests/ProcessStartOptionsTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/ProcessStartOptionsTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/ProcessStartOptionsTests.cs
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/ProcessStartOptionsTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/ProcessStartOptionsTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartOptions.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartOptions.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartOptions.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/ProcessStartOptionsTests.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/ProcessStartOptionsTests.Unix.cs
Show resolved
Hide resolved
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot address my feedback
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartOptions.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartOptions.cs
Show resolved
Hide resolved
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot please address my feedback
src/libraries/System.Diagnostics.Process/tests/ProcessStartOptionsTests.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/ProcessStartOptionsTests.Windows.cs
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/ProcessStartOptionsTests.Windows.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/ProcessStartOptionsTests.Windows.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/ProcessStartOptionsTests.Windows.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/ProcessStartOptionsTests.Windows.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/ProcessStartOptionsTests.Windows.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/ProcessStartOptionsTests.Windows.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/ProcessStartOptionsTests.Windows.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartOptions.cs
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartOptions.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartOptions.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartOptions.cs
Show resolved
Hide resolved
- add comments - cache Environment.SystemDirectory - handle fully qualified paths on Windows
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartOptions.cs
Outdated
Show resolved
Hide resolved
|
@copilot invoke the code-review skill and post your analysis/results as a comment on this pr |
| // "Applications should not under any circumstances pass user-controlled input directly to CreateProcess; | ||
| // or, if they intend to do so (for passing user-originated parameters), they should be filling out the lpApplicationName parameter." | ||
| // | ||
| // We could either document that the FileName should never be user-controlled input or resolve it ourselves, |
There was a problem hiding this comment.
I do not understand this point. If we wanted to leave resolution of the file name to the OS, we could add the quotation marks around the file name as the official documentation suggests.
I think the main problem with leaving the resolution to the OS is that it would not work well with the current public API shape. We would not be able to provide the resolved path before the process starts as a public OS neutral API.
So I think the reasoning should be as follows:
- We want to provide resolved path API
- It means we need to have our own copy of the path resolution algorithm since there is no Windows OS API to do that.
- Our own copy of the path resolution algorithm is not going to be equivalent to the Windows OS provided one since it is not fully documented.
- We take it as an opportunity to clean up the legacy baggage to have simpler, easier to understand and more secure filename resolution algorithm that is consistent across OSes and aligned with other modern platforms.
There was a problem hiding this comment.
I do not understand this point. If we wanted to leave resolution of the file name to the OS, we could add the quotation marks around the file name as the official documentation suggests.
This was also my understanding, but the feedback I got from the Windows Team made me believe we should not be doing this.
- We take it as an opportunity to clean up the legacy baggage to have simpler, easier to understand and more secure filename resolution algorithm that is consistent across OSes and aligned with other modern platforms.
I agree it's great opportunity and timing (Preview 2?). However, which parts exactly do we want to give up on?
no current directory probing
For CWD, we could just explicitly demand the ".\here.exe" syntax.
no process .exe path probing
It's a valid scenario, but the users can use Path.Combine(Path.GetDirectoryName(Environment.ProcessPath), "name") API to build such path quickly (assuming Environment.ProcessPath does not return null).
no legacy Windows directory probing, etc.
Just to be sure, by legacy you don't mean C:\Windows\System32 (returned by Environment.ProcessPath, contains "cmd.exe" etc), but C:\Windows\System?
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot Please address my feedback
src/libraries/Common/src/Interop/Windows/Kernel32/Interop.GetWindowsDirectoryW.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartOptions.cs
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartOptions.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartOptions.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/ProcessStartOptionsTests.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/ProcessStartOptionsTests.Windows.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/ProcessStartOptionsTests.Windows.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/ProcessStartOptionsTests.Windows.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Head branch was pushed to by a user without write access
|
@copilot address all of my feedback, not only some parts of it |
… explicit ./ Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
| if (path != null) | ||
| { | ||
| path = Path.Combine(path, filename); | ||
| if (File.Exists(path)) | ||
| { | ||
| return path; | ||
| } |
There was a problem hiding this comment.
Environment.SystemDirectory is a non-nullable string property that always returns a value. The null check on line 247 is unnecessary and violates the coding guideline to trust C# null annotations and avoid null checks when the type system indicates a value cannot be null.
| if (path != null) | |
| { | |
| path = Path.Combine(path, filename); | |
| if (File.Exists(path)) | |
| { | |
| return path; | |
| } | |
| path = Path.Combine(path, filename); | |
| if (File.Exists(path)) | |
| { | |
| return path; |
Description
Adds
ProcessStartOptionsclass providing explicit, simplified path resolution for process creation. Addresses security and predictability concerns with automatic directory probing.Changes
API Surface
ProcessStartOptions(string fileName)- constructor resolves and validates executable pathsFileName,Arguments,Environment,WorkingDirectory,InheritedHandles,KillOnParentExit,CreateNewProcessGroupArguments,Environment,InheritedHandles)Environment.GetEnvironmentVariables()asDictionary<string, string?>Path Resolution Behavior
.\prefix../prefix.C:\path\file.exe) returned as-isC:file.exe) normalized viaPath.GetFullPath.exeappended when filename lacks extension and path separatorsFileNotFoundExceptionwith localized message when resolution failsImplementation Details
Environment.SystemDirectoryinstead of P/InvokeProcess.IsExecutableinternal for shared useTests
.\file, negative:file)IsNotWindowsNanoServer,IsNotWindowsServerCore)EndsWithandPath.GetFullPath()for cross-platform robustnessExample
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.